Re-introduce concept of projection cache 'completion' #89831

Aaron1011 · 2021-10-12T21:32:56Z

Instead of clearing out the cache entirely, we store
the intermediate evaluation result into the cache entry.
This accomplishes several things:

We avoid the performance hit associated with re-evaluating
the sub-obligations
We avoid causing issues with incremental compilation, since
the final evaluation result is always the same
We avoid affecting other uses of the same InferCtxt which
might care about 'side effects' from processing the sub-obligations
(e,g. region constraints). Only code that is specifically aware
of the new 'complete' code is affected

Authored to address issue #89195

rust-highfive · 2021-10-12T21:32:59Z

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

Aaron1011 · 2021-10-12T21:33:08Z

@bors try @rust-timer queue

rust-timer · 2021-10-12T21:33:09Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-10-12T21:33:16Z

⌛ Trying commit 58de3037235c48765dcdfb6530cb316ac025ec5b with merge fa1210416b5caa99212b19d594ef3da8ae3132f1...

bors · 2021-10-12T22:58:04Z

☀️ Try build successful - checks-actions
Build commit: fa1210416b5caa99212b19d594ef3da8ae3132f1 (fa1210416b5caa99212b19d594ef3da8ae3132f1)

rust-timer · 2021-10-12T22:58:05Z

Queued fa1210416b5caa99212b19d594ef3da8ae3132f1 with parent 0446743, future comparison URL.

rust-timer · 2021-10-13T01:18:44Z

Finished benchmarking commit (fa1210416b5caa99212b19d594ef3da8ae3132f1): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -6.0% on incr-unchanged builds of regression-31157)
Very large regression in instruction counts (up to 19.6% on incr-unchanged builds of deeply-nested)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

nayato · 2021-10-13T05:16:53Z

This fixes enormous regression we've seen introduced seemingly with #85868.
Debug build before before #85868: 3 min
Same build after #85868: 12+ h - at which point we don't wait for it anymore and abort it.
Same build with this PR: 3.5 min

Aaron1011 · 2021-10-13T05:26:49Z

@nayato: I'm glad to hear it! I came up with this as a fix for #89195, which appears to have the same underlying cause as your issue.

Unfortunately, part of the issue with #89195 seems to be be the 'eager' evaluation and discarding logic that I added in #85868 (and that I've now removed from this PR). When there are a large number of projections with a large number of sub-obligations, this appears to blow up compilation time. Unfortunately, removing that logic appears to have a large negative impact on the deeply-nested benchmark.

Aaron1011 · 2021-10-13T18:43:48Z

@bors try @rust-timer queue

rust-timer · 2021-10-13T18:43:50Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-10-13T18:43:57Z

⌛ Trying commit 6971dea4036a802fe9d7a838b8d3e655fee7d70f with merge 38dc5847c0733779aaaa9e3de4408165ea7a2b05...

jackh726 · 2021-10-13T19:06:21Z

Perf aside here, I would have extreme reservations about landing this PR without proper tests for the original incremental issues that required us to remove a lot of the caching code.

bors · 2021-10-13T20:05:22Z

☀️ Try build successful - checks-actions
Build commit: 38dc5847c0733779aaaa9e3de4408165ea7a2b05 (38dc5847c0733779aaaa9e3de4408165ea7a2b05)

rust-timer · 2021-10-13T20:05:24Z

Queued 38dc5847c0733779aaaa9e3de4408165ea7a2b05 with parent 81117ff, future comparison URL.

rust-timer · 2021-10-13T22:45:23Z

Finished benchmarking commit (38dc5847c0733779aaaa9e3de4408165ea7a2b05): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Very large improvement in instruction counts (up to -6.0% on incr-unchanged builds of regression-31157)
Very large regression in instruction counts (up to 19.8% on incr-unchanged builds of deeply-nested)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

bors · 2021-12-18T20:26:48Z

☀️ Try build successful - checks-actions
Build commit: 02aeffa72b11c0501c63a17d3e3f27c833ef2cbf (02aeffa72b11c0501c63a17d3e3f27c833ef2cbf)

rust-timer · 2021-12-18T20:26:50Z

Queued 02aeffa72b11c0501c63a17d3e3f27c833ef2cbf with parent d3848cb, future comparison URL.

rust-timer · 2021-12-18T23:58:55Z

Finished benchmarking commit (02aeffa72b11c0501c63a17d3e3f27c833ef2cbf): comparison url.

Summary: This change led to small relevant mixed results 🤷 in compiler performance.

Small improvement in instruction counts (up to -1.2% on full builds of regression-31157)
Small regression in instruction counts (up to 0.5% on incr-unchanged builds of webrender-wrench)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instead of clearing out the cache entirely, we store the intermediate evaluation result into the cache entry. This accomplishes several things: * We avoid the performance hit associated with re-evaluating the sub-obligations * We avoid causing issues with incremental compilation, since the final evaluation result is always the same * We avoid affecting other uses of the same `InferCtxt` which might care about 'side effects' from processing the sub-obligations (e,g. region constraints). Only code that is specifically aware of the new 'complete' code is affected

Aaron1011 · 2021-12-19T00:08:25Z

@bors r=jackh726

bors · 2021-12-19T00:08:26Z

📌 Commit 40ef1d3 has been approved by jackh726

bors · 2021-12-19T03:33:22Z

⌛ Testing commit 40ef1d3 with merge d6cffe4...

bors · 2021-12-19T06:26:00Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing d6cffe4 to master...

rust-timer · 2021-12-19T09:05:32Z

Finished benchmarking commit (d6cffe4): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

matze · 2021-12-21T07:54:11Z

Any chance this can make it into 1.58? It fixes severe build times we have since 1.56 in a tonic/tower-based application of ours and essentially holding us back from upgrading to edition 2021 and a more recent rustls.

jackh726 · 2021-12-21T15:34:39Z

I'll nominate this for stable and beta.

Personally, I think is at least as risky as #90423, and that got stable declined. But a beta backport might be okay here. I'll nominate both for discussion.

pnkfelix · 2021-12-23T15:27:07Z

Discussed in T-compiler meeting. Deferring decision on whether to backport (any of beta/stable) until next week, at least.

apiraino · 2021-12-30T16:01:05Z

Discussed again today and the decision is to not baclport this on both beta or stable channels Zulip topic

@rustbot label -beta-nominated -stable-nominated

rust-highfive assigned wesleywiser Oct 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 12, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 13, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 13, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 13, 2021

This comment has been minimized.

Sign in to view

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021

This comment has been minimized.

Sign in to view

Aaron1011 force-pushed the project-caching-speedup branch from 6971dea to 62e7bcd Compare October 18, 2021 18:49

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2021

Aaron1011 force-pushed the project-caching-speedup branch from 2517bd2 to 40ef1d3 Compare December 19, 2021 00:07

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2021

bors merged commit d6cffe4 into rust-lang:master Dec 19, 2021

rustbot added this to the 1.59.0 milestone Dec 19, 2021

rustbot removed the perf-regression Performance regression. label Dec 19, 2021

Aaron1011 mentioned this pull request Dec 19, 2021

Add minimized version of projection caching example rust-lang/rustc-perf#1124

Closed

jackh726 added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Dec 21, 2021

rustbot removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Dec 30, 2021

wesleywiser mentioned this pull request Jan 18, 2022

Add benchmark for projection caching issue rust-lang/rustc-perf#1153

Merged

Aaron1011 mentioned this pull request Jan 20, 2022

rustc does not terminate after 1.57.0 #93137

Closed

dingelish mentioned this pull request Jan 20, 2022

Compilation of heim-virt 0.0.11 is hanging heim-rs/heim#356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-introduce concept of projection cache 'completion' #89831

Re-introduce concept of projection cache 'completion' #89831

Aaron1011 commented Oct 12, 2021 •

edited by pnkfelix

Loading

rust-highfive commented Oct 12, 2021

Aaron1011 commented Oct 12, 2021

rust-timer commented Oct 12, 2021

bors commented Oct 12, 2021

bors commented Oct 12, 2021

rust-timer commented Oct 12, 2021

rust-timer commented Oct 13, 2021

nayato commented Oct 13, 2021

Aaron1011 commented Oct 13, 2021

Aaron1011 commented Oct 13, 2021

rust-timer commented Oct 13, 2021

bors commented Oct 13, 2021

jackh726 commented Oct 13, 2021

bors commented Oct 13, 2021

rust-timer commented Oct 13, 2021

rust-timer commented Oct 13, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Dec 18, 2021

rust-timer commented Dec 18, 2021

rust-timer commented Dec 18, 2021

Aaron1011 commented Dec 19, 2021

bors commented Dec 19, 2021

bors commented Dec 19, 2021

bors commented Dec 19, 2021

rust-timer commented Dec 19, 2021

matze commented Dec 21, 2021

jackh726 commented Dec 21, 2021 •

edited

Loading

pnkfelix commented Dec 23, 2021

apiraino commented Dec 30, 2021

Re-introduce concept of projection cache 'completion' #89831

Re-introduce concept of projection cache 'completion' #89831

Conversation

Aaron1011 commented Oct 12, 2021 • edited by pnkfelix Loading

rust-highfive commented Oct 12, 2021

Aaron1011 commented Oct 12, 2021

rust-timer commented Oct 12, 2021

bors commented Oct 12, 2021

bors commented Oct 12, 2021

rust-timer commented Oct 12, 2021

rust-timer commented Oct 13, 2021

nayato commented Oct 13, 2021

Aaron1011 commented Oct 13, 2021

Aaron1011 commented Oct 13, 2021

rust-timer commented Oct 13, 2021

bors commented Oct 13, 2021

jackh726 commented Oct 13, 2021

bors commented Oct 13, 2021

rust-timer commented Oct 13, 2021

rust-timer commented Oct 13, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Dec 18, 2021

rust-timer commented Dec 18, 2021

rust-timer commented Dec 18, 2021

Aaron1011 commented Dec 19, 2021

bors commented Dec 19, 2021

bors commented Dec 19, 2021

bors commented Dec 19, 2021

rust-timer commented Dec 19, 2021

matze commented Dec 21, 2021

jackh726 commented Dec 21, 2021 • edited Loading

pnkfelix commented Dec 23, 2021

apiraino commented Dec 30, 2021

Aaron1011 commented Oct 12, 2021 •

edited by pnkfelix

Loading

jackh726 commented Dec 21, 2021 •

edited

Loading